Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mono.Android] Java.Lang.Object.GetObject<T>() implementation #9630

Closed
wants to merge 2 commits into from

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Dec 18, 2024

The current implementation:

return Java.Interop.TypeManager.CreateInstance (handle, transfer, type);

Needs to become this, for NativeAOT support:

JniObjectReference reference = JNIEnv.CreateJniObjectReference (handle, transfer);
JniObjectReferenceOptions options = JNIEnv.ToJniObjectReferenceOptions (transfer);
return (IJavaPeerable) JNIEnvInit.AndroidValueManager?.GetValue (ref reference, options, type);

This way we use the appropriate AndroidValueManager for the runtime
that is currently being used. If we just change the code in
Java.Lang.Object to do this for all runtimes, this can keep
everything a bit simpler.

The fallout of this change requires type to be decorated with:

[DynamicallyAccessedMembers (Constructors)]

This trickled to create many other trimmer warnings, that are likely a
real issue. This is a good opportunity to fix them.

I also defined the const value in Java.Lang.Object:

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

Which allowed many types that extend Java.Lang.Object able to reuse
this const value and not define it again.

I also had to make a helper function to map JniHandleOwnership ->
JniObjectReferenceOptions, such as:

internal static JniObjectReferenceOptions ToJniObjectReferenceOptions (JniHandleOwnership transfer)

And CreateJniObjectReference():

internal static JniObjectReference CreateJniObjectReference (IntPtr handle, JniHandleOwnership transfer)

Other changes:

Changes: dotnet/java-interop@f800ea5...2c06b3c

Comment on lines 707 to 736
return Java.Lang.Object.GetObject (elem, JniHandleOwnership.TransferLocalRef, type);
return GetObject (elem, type);

// FIXME: https://github.com/xamarin/xamarin-android/issues/8724
// Since a Dictionary<Type, Func> is used here, the trimmer will not be able to properly analyze `Type t`
// error IL2111: Method 'lambda expression' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method.
[UnconditionalSuppressMessage ("Trimming", "IL2067", Justification = "[DynamicallyAccessedMembers] manually added to all callers")]
static object? GetObject (IntPtr e, Type t) =>
Java.Lang.Object.GetObject (e, JniHandleOwnership.TransferLocalRef, t);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find a way to fix this properly yet. I think the pattern of:

  • Create a Dictionary<Type, Func> where one of the parameters needs [DynamicallyAccessedMembers]
  • If you decorate the lambda, or create a method...
  • The trimmer says: "Method 'lambda expression' with parameters or return value with DynamicallyAccessedMembersAttribute is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method."

This situation just isn't going to make the trimmer happy.

For now, I linked to:

The future fix might be to remove the Dictionary in favor of switch, that seems like the trimmer could more easily understand.

@jonathanpeppers jonathanpeppers force-pushed the dev/peppers/AndroidValueManager branch from eff56dd to cc52218 Compare December 18, 2024 15:17
jonathanpeppers added a commit to dotnet/java-interop that referenced this pull request Dec 18, 2024
Context: dotnet/android#9630

As #9630 required new declarations of
`DynamicallyAccessedMemberTypes.Interfaces`, we intestigated to see
*why* these were needed in Java.Interop.

There are two cases:

    var listIface   = typeof (IList<>);
    var listType    =
        (from iface in type.GetInterfaces ().Concat (new[]{type})
            where (listIface).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface)
            select iface)
        .FirstOrDefault ();

This first one, if `IList<>` were trimmed away. We don't actually
care, as everything in this code path would still work. We can
suppress the warning instead.

The second case:

    JniValueMarshalerAttribute? ifaceAttribute = null;
    foreach (var iface in type.GetInterfaces ()) {
        marshalerAttr = iface.GetCustomAttribute<JniValueMarshalerAttribute> ();
        if (marshalerAttr != null) {
            if (ifaceAttribute != null)
                throw new NotSupportedException ($"There is more than one interface with custom marshaler for type {type}.");

            ifaceAttribute = marshalerAttr;
        }
    }
    if (ifaceAttribute != null)
        return (JniValueMarshaler) Activator.CreateInstance (ifaceAttribute.MarshalerType)!;

Feels like we should be able to remove this code completely.

With these changes we can remove `DynamicallyAccessedMemberTypes.Interfaces`.

I also introduced `build-tools/trim-analyzers/trim-analyzers.props`
that will setup the appropriate trimmer MSBuild properties to make
trimmer warnings an error. This should keep us from accidentally
creating warnings. I only use this setting in projects that were
already using `$(EnableAotAnalyzer)`.
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Dec 18, 2024
)

Context: dotnet/android#9630
Context: ec2813a

As dotnet/android#9630 required new declarations of
`DynamicallyAccessedMemberTypes.Interfaces`, we intestigated to see
*why* these were needed in Java.Interop.

There are two cases that involve `.Interfaces` within 
`JniRuntime.JniValueManager.GetValueMarshaler(Type)`:

The first case is around attempts to special-case array marshaling
for builtin types:

	var listIface   = typeof (IList<>);
	var listType    =
	    (from iface in type.GetInterfaces ().Concat (new[]{type})
	        where (listIface).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface)
	        select iface)
	    .FirstOrDefault ();

If `IList<T>` is trimmed away (lol), then *we don't care*, as
everything else within `GetValueMarshaler(Type)` still works,
and if we can't get a value marshaler for `int[]` (which implements
`IList<T>`!), then so be it.  Suppress this IL2070 message.

The second case is around looking for `[JniValueMarshaler]` on
implemented interface types, from ec2813a:

	JniValueMarshalerAttribute? ifaceAttribute = null;
	foreach (var iface in type.GetInterfaces ()) {
	    marshalerAttr = iface.GetCustomAttribute<JniValueMarshalerAttribute> ();
	    if (marshalerAttr != null) {
	        if (ifaceAttribute != null)
	            throw new NotSupportedException ($"There is more than one interface with custom marshaler for type {type}.");

	        ifaceAttribute = marshalerAttr;
	    }
	}
	if (ifaceAttribute != null)
	    return (JniValueMarshaler) Activator.CreateInstance (ifaceAttribute.MarshalerType)!;

It is to support the scenario

	namespace Android.Runtime {
	    [JniValueMarshaler(typeof(IJavaObjectValueMarshaler))]
	    partial interface /* Android.RuntimIJavaObject {}
	}
	namespace Java.Lang {
	    partial class Object : Android.Runtime.IJavaObject {}
	}

to "retrofit" `Java.Lang.Object` and `Java.Lang.Throwable` to use
`IJavaObjectValueMarshaler`.

There are three problems with this case:

 1. It's not actually covered by unit tests!  Removing the above
    `foreach` loop doesn't break any unit tests.

 2. While dotnet/android was updated for this scenario, it was only
    as part of trying to use `jnimarshalmethod-gen` on
    Xamarin.Android projects, which was never finished.

 3. It doesn't "scale": the code throws a `NotSupportedException`
    if a type implements more than one interface that has
    `[JniValueMarshaler]`.

This isn't used, and *shouldn't* be relied upon.  Excise it entirely.

With these changes we can remove
`DynamicallyAccessedMemberTypes.Interfaces`.

I also introduced `build-tools/trim-analyzers/trim-analyzers.props`
that will setup the appropriate trimmer MSBuild properties to make
trimmer warnings an error.  This should keep us from accidentally
creating warnings.  I only use this setting in projects that were
already using `$(EnableAotAnalyzer)`.
@jonathanpeppers jonathanpeppers force-pushed the dev/peppers/AndroidValueManager branch from 6cc1754 to 6398552 Compare December 18, 2024 21:47
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 18, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 18, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 18, 2024
The current implementation:

    return Java.Interop.TypeManager.CreateInstance (handle, transfer, type);

Needs to become this, for NativeAOT support:

    JniObjectReference reference = JNIEnv.CreateJniObjectReference (handle, transfer);
    JniObjectReferenceOptions options = JNIEnv.ToJniObjectReferenceOptions (transfer);
    return (IJavaPeerable) JNIEnvInit.AndroidValueManager?.GetValue (ref reference, options, type);

This way we use the appropriate `AndroidValueManager` for the runtime
that is currently being used. If we just change the code in
`Java.Lang.Object` to do this for all runtimes, this can keep
everything a bit simpler.

The fallout of this change requires `type` to be decorated with:

    [DynamicallyAccessedMembers (Constructors)]

This trickled to create many other trimmer warnings, that are likely a
real issue. This is a good opportunity to fix them.

I also defined the `const` value in `Java.Lang.Object`:

    internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

Which allowed many types that extend `Java.Lang.Object` able to reuse
this `const` value and not define it again.

I also had to make a helper function to map `JniHandleOwnership` ->
`JniObjectReferenceOptions`, such as:

    internal static JniObjectReferenceOptions ToJniObjectReferenceOptions (JniHandleOwnership transfer)

And `CreateJniObjectReference()`:

    internal static JniObjectReference CreateJniObjectReference (IntPtr handle, JniHandleOwnership transfer)

Other changes:

* Bump to dotnet/java-interop@2c06b3c2a

Changes: dotnet/java-interop@f800ea5...2c06b3c
@jonathanpeppers jonathanpeppers force-pushed the dev/peppers/AndroidValueManager branch from 6398552 to 2fea602 Compare December 18, 2024 21:58
@dotnet dotnet deleted a comment from azure-pipelines bot Dec 18, 2024
@jonathanpeppers
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

return Java.Interop.TypeManager.CreateInstance (handle, transfer, type);
JniObjectReference reference = JNIEnv.CreateJniObjectReference (handle, transfer);
JniObjectReferenceOptions options = JNIEnv.ToJniObjectReferenceOptions (transfer);
return (IJavaPeerable) JNIEnvInit.AndroidValueManager?.GetValue (ref reference, options, type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong about always wanting this cast.

That said, IIRC a previous version of this PR instead used as IJavaPeerable, which also seems wrong.

The problem: Java.InteropTests.JnienvTest.MoarThreadingTests is failing:

No exception should be thrown [t2]! Got: System.InvalidCastException: Arg_InvalidCastException
at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()
Expected: null
But was:  <System.InvalidCastException: Arg_InvalidCastException
at Java.Lang.Object.GetObject(IntPtr , JniHandleOwnership , Type )
at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|74_11(IntPtr , Type )
at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__74_9(Type type, IntPtr source, Int32 index)
at Android.Runtime.JNIEnv.GetObjectArray(IntPtr , Type[] )
at Java.InteropTests.JnienvTest.<>c__DisplayClass26_0.<MoarThreadingTests>b__1()>

IntPtr lrefJliArray = JNIEnv.NewObjectArray<int> (new[]{1});
IntPtr grefJliArray = JNIEnv.NewGlobalRef (lrefJliArray);
JNIEnv.DeleteLocalRef (lrefJliArray);
Exception ignore_t1 = null;
Exception ignore_t2 = null;
var t1 = new Thread (() => {
int[] output_array1 = new int[1];
for (int i = 0; i < 2000; ++i) {
Console.WriteLine ("# t1 iter: {0}", i);
try {
JNIEnv.CopyObjectArray (grefJliArray, output_array1);
} catch (Exception e) {
ignore_t1 = e;
break;
}
}
});
var t2 = new Thread (() => {
for (int i = 0; i < 2000; ++i) {
Console.WriteLine ("# t2 iter: {0}", i);
try {
JNIEnv.GetObjectArray (grefJliArray, new[]{typeof (int)});

JnienvTest.cs:386 is the source of the JNIEnv.GetObjectArray() invocation at the root of the stack trace.

The cast on this line is the source of the InvalidCastException.

The question is: how did this previously work, and why is it failing now?

Tracing through how it previously worked:

  1. JNIEnv.GetObjectArray() is called; element_types=new[]{typeof(int)}.
  2. JNIEnv.GetObjectArray() calls GetConverter<T>(NativeArrayElementToManaged, null, array_ptr)
  3. GetConverter<T>() sees that array_ptr/array is not null; calls GetClassNameFromInstance(array).
  4. GetClassNameFromInstance(array) should be [Ljava/lang/Object;, because the array was created via JNIEnv.NewObjectArray<int> (new[]{1}).
  5. GetConverter<T>() thus skips the switch on lines 732-749
  6. GetConvter<T>() should thus return a Func<Type?, IntPtr, int, object?> for IJavaObject:
    { typeof (IJavaObject), (type, source, index) => {
    AssertIsJavaObject (type);
    IntPtr elem = GetObjectArrayElement (source, index);
    return Java.Lang.Object.GetObject (elem, JniHandleOwnership.TransferLocalRef, type);
    } },
  7. Returning to GetObjectArray(), line 1110 targetType will be typeof(int), we call converter(null, array_ptr, i).
  8. value should thus be a java.lang.Integer/Java.Lang.Integer instance, and the Convert.ChangeType() on line should turn it into an int, as Integer implements IConvertible.

So why's this change fail? Probably because AndroidValueManager?.GetValue(…) has different semantics from TypeManager.CreateInstance(…). I need to further investigate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonpryor wrote:

So why's this change fail? Probably because AndroidValueManager?.GetValue(…) has different semantics from TypeManager.CreateInstance(…). I need to further investigate.

Time for further investigation! Consider the following patch to java-interop/samples/Hello-Java.Base:

diff --git a/samples/Hello-Java.Base/Program.cs b/samples/Hello-Java.Base/Program.cs
index b1e0d5ba..9e63f3ba 100644
--- a/samples/Hello-Java.Base/Program.cs
+++ b/samples/Hello-Java.Base/Program.cs
@@ -61,10 +61,15 @@ namespace Hello
 			CreateJLO ();
 		}
 
-		static void CreateJLO ()
+		static unsafe void CreateJLO ()
 		{
-			var jlo = new Java.Lang.Object ();
-			Console.WriteLine ($"binding? {jlo.ToString ()}");
+			var i_class = new JniType ("java/lang/Integer");
+			var i_ctor  = i_class.GetConstructor ("(I)V");
+			JniArgumentValue* i_args = stackalloc JniArgumentValue [1];
+			i_args [0] = new JniArgumentValue (42);
+			var i_value = i_class.NewObject (i_ctor, i_args);
+			var v = JniEnvironment.Runtime.ValueManager.GetValue (ref i_value, JniObjectReferenceOptions.CopyAndDispose, null);
+			Console.WriteLine ($"v? {v} {v?.GetType ()}");
 		}
 
 		static void ReportTiming ()

This creates a new java.lang.Integer(42), invokes Runtime.ValueManager.GetValue(), and prints the resulting value and its type.

The output is:

% ~/Downloads/dotnet-sdk-8.0.206-osx-x64/dotnet run --project samples/Hello-Java.Base/*.csproj
…
v? 42 System.Int32

(I have to use .NET 8.0.206 because anything later crashes. Hopefully this will be fixed in the January .NET 8 service release…)

Note that Runtime.GetValue(objref, …, null) converts to a System.Int32, not a Java.Lang.Integer. This is why there's an InvalidCastException.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the cause of the InvalidCastException is understood, what do we do about it?

There are at least two paths worth pursuing:

  1. Don't Do That™ (don't use ValueManager.GetValue() here)
  2. Don't Do That™ (update ValueManager.GetValue() to not auto-unbox like that.)

(1) should be possible, by e.g. updating TypeManager.CreateInstance() to use methods on JniEnvironment.Runtime.TypeManager instead of using the P/Invokes within GetClassName()/etc.

(2) should also be possible, but may be more time consuming, depending on how many unit tests it breaks.

jonpryor pushed a commit that referenced this pull request Dec 19, 2024
Context: 8d77130
Context: #9630

We're exploring how to get .NET for Android apps to build and run
using [Native AOT][0].

Default to `$(TrimMode)=Full` for NativeAOT, this enables trimmer
warnings and should be the default mode for NativeAOT.

Ensure the `_PrepareLinking` MSBuild target runs at the appropriate
time.  Failure to do so was causing
`Android.App.Activity.GetOnCreate_Landroid_os_Bundle_Handler()` to be
trimmed away.

Various fixes to avoid `java.lang.UnsatisfiedLinkError` errors:

  * Set `$(LinkerFlavor)=lld` by default to avoid:

        E AndroidRuntime: java.lang.UnsatisfiedLinkError: dlopen failed: cannot locate symbol "__start___modules" referenced by
          "/data/app/~~_ggpMC4foLk_jUUycm0CfA==/net.dot.hellonativeaot-fvszIWroqgweLHYgULxVoQ==/split_config.arm64_v8a.apk!/lib/arm64-v8a/libNativeAOT.so"...

  * Include `libc++_shared.so` within the app, as Native AOT output
    requires C++:

        E AndroidRuntime: java.lang.UnsatisfiedLinkError: dlopen failed: library "libc++_shared.so" not found: needed by
          /data/app/~~_W0B9EE3hhajnFvCHyUKSg==/net.dot.hellonativeaot-zlXemqHdkbHaLu60oYPVQQ==/lib/arm64/libNativeAOT.so in namespace clns-6

Emit `JavaPeerStyle.JavaInterop1` java stubs for NativeAOT, as it is
easier to get working in place of `JavaPeerStyle.XAJavaInterop1`.
Specifically, we need to be able to avoid P/Invokes related to
typemaps/etc.  XAJavaInterop1 hits `JNIEnvInit.RegisterJniNatives()`,
which is full of P/Invokes such as `TypeManager.GetClassName()`,
while `JavaInterop1` hits `ManagedPeer.RegisterNativeMembers()` which
goes through `JniRuntime` abstractions, allowing for a P/Invoke-free
code path.

I updated `BuildTest2.NativeAOT()` to assert for these changes where
possible.

[0]: https://learn.microsoft.com/dotnet/core/deploying/native-aot/
@jonathanpeppers
Copy link
Member Author

This does not appear to be needed for "Hello World" in #9636, so we'll revisit this later.

It will likely be needed if we run Mono.Android-Tests under NativeAOT.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants